Fix hybrid scan parquet reader incorrectly parse bloom filter - #22901
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
ca7c8b0 to
5acd266
Compare
399b73b to
ccf7988
Compare
…; remove outdated test for Bloom filters with real data Need to rebuild all the code for A/B test
… read_parquet Updated the test for hybrid scan bloom filtering to explicitly compare its behavior against the read_parquet method. Enhanced debug logging to provide detailed output of the bloom filter data being processed, ensuring consistency in row group retention between the two methods. Adjusted comments and structure for better readability and understanding of the test's purpose.
Refactored the hybrid scan bloom filter test to utilize parameterized fixtures for improved flexibility and clarity. This change allows for multiple test cases to be run with different input values, enhancing the robustness of the test against various scenarios. Updated the test structure and comments for better readability and understanding of the A/B comparison with the read_parquet method.
…and clarity Updated the hybrid scan implementation to replace the previous method of fetching bloom filter data with a new function, `fetch_bloom_filters_to_device_async`, which strips headers and ensures 32-byte alignment. This change enhances the efficiency of bloom filter processing and simplifies the codebase. Additionally, updated related tests to reflect these changes and ensure consistency in behavior across different methods of bloom filter handling.
ccf7988 to
df8fc33
Compare
Removed unused imports and functions related to bloom filter processing in the hybrid scan module. This cleanup enhances code clarity and reduces complexity. Updated the test suite to reflect these changes, ensuring that the functionality remains intact and consistent with previous implementations.
Modified copyright statements in multiple files to include "NVIDIA CORPORATION & AFFILIATES." Enhanced code readability by breaking long parameter descriptions into multiple lines in the `parquet_io_utils.hpp` file. Removed outdated debug logging and unnecessary comments in the `bloom_filter_reader.cu` and `parquet_io_utils.cpp` files to streamline the codebase.
…vice_async_impl` Simplified comments and improved code clarity by removing outdated explanations regarding bloom filter header handling. The changes focus on enhancing readability while maintaining the functionality of fetching bloom filter data to the device. This aligns with previous efforts to streamline the hybrid scan implementation and improve performance.
…gement - Removed unnecessary `copy_dsts` vector and related null pointer handling. - Simplified the logic for populating `copy_srcs` and `copy_sizes`. - Ensured that device memory allocation is only performed when `device_base` is not null. - Improved clarity and maintainability of the code by restructuring deferred read handling. This change enhances the efficiency of the bloom filter fetching process in the Parquet I/O utilities.
mhaseeb123
left a comment
There was a problem hiding this comment.
Few optional non-blocking comments but LGTM otherwise. Would recommend removing the Follow-up PR section from the description and instead creating an issue for better tracking. I didn't understand what "Edge cases for current bloom filter fetching function" means? Are there any unhandled edge cases in this PR?
Thanks for the review. There are no known unhandled edge cases in this PR. “Edge cases for the current bloom filter fetching function” refers to follow-up pytest coverage for scenarios that the committed Parquet fixtures do not cover well. I’ve removed the Follow-up PR section and opened #23393 to track this work instead. |
vuule
left a comment
There was a problem hiding this comment.
Looks good, few small suggestions.
…filters_to_device_impl`. Updated warning message to include dynamic block size and refined mutex lock scope for better readability.
|
/merge |
Description
Closes #22933
This PR fixes bloom filtering in the hybrid-scan Parquet reader. The previous path treated the full bloom filter payload (header + bitset) as a raw bitset; the new path fetches the filter to host, parses the header, and copies only the bitset to device. It also refactors
read_parquetand hybrid scan to share the same bloom-filter fetch path.Checklist